Skip to content

Conversation

@Critsium-xy
Copy link
Collaborator

@Critsium-xy Critsium-xy commented Jan 16, 2025

After Abacus start using template to implement caculation on different devices, the ctx parameter in operators became fully useless. But it is never removed because of its heavy usage. This pull request removes the ctx parameters in all memory operators.

Hint: If this pr is accepted, other pull requests may need rerun to ensure they are correct.

@Qianruipku
Copy link
Collaborator

It may be necessary to have a unified naming convention to distinguish whether this is a device or CPU resize_memory?

@Critsium-xy
Copy link
Collaborator Author

It may be necessary to have a unified naming convention to distinguish whether this is a device or CPU resize_memory?

In fact every type of operators have its own unique name defined by using keyword in memory_op.h, which clearly shows an operator's data type and device type. But no one uses that in fact. I used it in my new prs.

image

These new functions are also available for this

image

By using AbacusDevice_t you can clearly see which device this operator is operating, but this is merged yesterday so no one used it temporarily.

@Critsium-xy Critsium-xy marked this pull request as draft January 16, 2025 07:13
@Critsium-xy Critsium-xy changed the title [Refactor] Remove all ctx parameters in resize_memory_op [Refactor] Remove all ctx parameters in resize_memory_op and set_memory_op Jan 16, 2025
@Critsium-xy Critsium-xy marked this pull request as ready for review January 16, 2025 07:32
@AsTonyshment
Copy link
Collaborator

Nice work! I'm currently struggling with including things like this in my code:

Device* ctx = {};
base_device::DEVICE_CPU* cpu_ctx = {};

I was wondering what the hack exactly does this do? It seems that these pointers are no longer used after a total refactoring.

@Critsium-xy Critsium-xy marked this pull request as draft January 16, 2025 08:46
@Critsium-xy Critsium-xy marked this pull request as ready for review January 16, 2025 10:37
@Critsium-xy Critsium-xy changed the title [Refactor] Remove all ctx parameters in resize_memory_op and set_memory_op [Refactor] Remove all ctx parameters in memory operators Jan 16, 2025
@Qianruipku
Copy link
Collaborator

It may be necessary to have a unified naming convention to distinguish whether this is a device or CPU resize_memory?

In fact every type of operators have its own unique name defined by using keyword in memory_op.h, which clearly shows an operator's data type and device type. But no one uses that in fact. I used it in my new prs.

image

These new functions are also available for this

image

By using AbacusDevice_t you can clearly see which device this operator is operating, but this is merged yesterday so no one used it temporarily.

No one uses the using names defined in memory_op.h because they are not compatible with templates. Therefore, it is necessary to add extra using names in the definition of the template class. My suggestion is that although each class needs to include this using name, there should be a unified naming convention to distinguish between CPU and device, as well as between complex and real types. This would improve the readability of the code.

@mohanchen mohanchen added Refactor Refactor ABACUS codes GPU & DCU & HPC GPU and DCU and HPC related any issues The Absolute Zero Reduce the "entropy" of the code to 0 labels Jan 17, 2025
@Critsium-xy
Copy link
Collaborator Author

Exactly. I will soon add that suggested naming convention in the document later. But it seemed that this problem has no relationship with this pull request I think? Because the ctx parameter is completely useless as a parameter. Many codes even directly use a same "ctx" variable in every place where a ctx parameter is needed, no matter it is a gpu operator or cpu operator. I just removed it from parameter, not removing it from the template.

@mohanchen mohanchen merged commit 60b40bf into deepmodeling:develop Jan 18, 2025
14 checks passed
@Critsium-xy Critsium-xy deleted the remove_ctx branch February 11, 2025 06:34
Fisherd99 pushed a commit to Fisherd99/abacus-BSE that referenced this pull request Mar 31, 2025
…g#5862)

* Remove all ctx parameters in resize_memory_op

* Small bug fix

* [pre-commit.ci lite] apply automatic fixes

* Remove all ctx parameters in set_memory_op

* Remove ctx parameters in sync_memory_op

* Remove ctx parameters in cast_memory_op

* Remove ctx parameter in delete_memory_op

---------

Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

GPU & DCU & HPC GPU and DCU and HPC related any issues Refactor Refactor ABACUS codes The Absolute Zero Reduce the "entropy" of the code to 0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants